-
Notifications
You must be signed in to change notification settings - Fork 835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(api): deprecate MetricAttributes and MetricAttributeValue #3406
Conversation
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3406 +/- ##
==========================================
- Coverage 93.27% 93.26% -0.02%
==========================================
Files 247 247
Lines 7348 7348
Branches 1512 1512
==========================================
- Hits 6854 6853 -1
- Misses 494 495 +1
|
Actually, now I see that for trace API those types are deprecated. /**
* @deprecated please use {@link Attributes}
*/
export type SpanAttributes = Attributes;
/**
* @deprecated please use {@link AttributeValue}
*/
export type SpanAttributeValue = AttributeValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to move away from either SpanAttributes or MetricAttributes in the API package and encourage implementations that depend on the newer API package to use the Attributes
instead. These types are identical so there should be no breaking changes.
E.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can migrate to Attributes
in all occurrences in the API package in a follow-up PR.
Just a small fix to the metrics API comment I spotted when reviewing open-telemetry/opentelemetry-js-contrib#1281Probably it's a missed copy-paste mistakedeprecate
MetricAttribute
andMetricAttributeValue
to align withSpanAttributes
andSpanAttributeValue
.I wonder if it should also be replaced in the api package where it is used (I think yes)